ENG-1121 endSyncTask error - wrong worker #621
Conversation
|
Updates to Preview Branch (eng-1121-endsynctask-error-wrong-worker) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThese changes extend the sync_info table with a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Key areas requiring attention:
Possibly related PRs
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/database/supabase/schemas/sync.sql (1)
127-134: Consider usinglast_success_startdirectly for consistency.For a newly inserted row,
last_success_startis NULL (no previous successful completion). The current query (lines 130-133) will also return NULL since the row was just inserted with status='active'.While functionally correct, the existing-row path returns
t_last_success_startdirectly (line 154). For consistency, the new-row path could simply return NULL without the query.IF s_id IS NOT NULL THEN -- totally new_row, I'm on the task -- return last time it was run successfully - SELECT max(last_task_start) INTO result FROM public.sync_info - WHERE sync_target = s_target - AND sync_function = s_function - AND status = 'complete'; - RETURN result; + RETURN NULL; -- new task has no previous successful run END IF;
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/database/src/dbTypes.ts(3 hunks)packages/database/supabase/migrations/20251216222945_sync_last_success_start.sql(1 hunks)packages/database/supabase/schemas/sync.sql(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefertypeoverinterfacein TypeScript
Use explicit return types for functions
Avoidanytypes when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability
Files:
packages/database/src/dbTypes.ts
🧠 Learnings (5)
📚 Learning: 2025-06-09T16:57:14.681Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 198
File: packages/database/supabase/migrations/20250605083319_alpha_upload.sql:157-337
Timestamp: 2025-06-09T16:57:14.681Z
Learning: Migration files in packages/database/supabase/migrations/ are historical snapshots that preserve database schema and functions as they existed at the time of creation. These files should not be updated to reflect current schema changes, even if they reference tables or columns that have since been modified or removed. Schema incompatibilities in migration files are expected and acceptable as they represent the valid state at the time of migration.
Applied to files:
packages/database/supabase/migrations/20251216222945_sync_last_success_start.sqlpackages/database/supabase/schemas/sync.sql
📚 Learning: 2025-05-20T03:04:21.602Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250512142307_sync_table.sql:20-21
Timestamp: 2025-05-20T03:04:21.602Z
Learning: Database migration files (like the SQL files in packages/database/supabase/migrations/) should not be modified after they're created, even to fix minor issues like unused variables. Issues should be addressed in newer schema files instead to maintain migration history integrity.
Applied to files:
packages/database/supabase/migrations/20251216222945_sync_last_success_start.sqlpackages/database/supabase/schemas/sync.sql
📚 Learning: 2025-05-22T23:50:23.771Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250513173724_content_concept_key.sql:37-60
Timestamp: 2025-05-22T23:50:23.771Z
Learning: For the discourse-graph project, database schema management focuses on the final state in the supabase/schemas directory, not on the individual migration files. When reviewing database changes, consider only the schema definitions in this directory, not potential duplications or conflicts across migration files.
Applied to files:
packages/database/supabase/migrations/20251216222945_sync_last_success_start.sqlpackages/database/supabase/schemas/sync.sql
📚 Learning: 2025-05-20T03:01:11.220Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504195841_remote_schema.sql:1-7
Timestamp: 2025-05-20T03:01:11.220Z
Learning: Supabase automatically creates necessary schemas like the "extensions" schema before migrations run, so there's no need to explicitly create these schemas in migration files.
Applied to files:
packages/database/supabase/migrations/20251216222945_sync_last_success_start.sqlpackages/database/supabase/schemas/sync.sql
📚 Learning: 2025-05-20T03:11:07.917Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504202930_content_tables.sql:37-45
Timestamp: 2025-05-20T03:11:07.917Z
Learning: When reviewing SQL migrations in this codebase, understand that historical migration files should not be modified once they're applied. Instead, issues should be fixed in subsequent migrations or schema definition files. This is why maparent indicated "migration, not changing. corrected in schema."
Applied to files:
packages/database/supabase/migrations/20251216222945_sync_last_success_start.sql
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Supabase Preview
🔇 Additional comments (6)
packages/database/src/dbTypes.ts (1)
639-680: LGTM!The auto-generated type definitions correctly reflect the new
last_success_startcolumn: nullable inRow, optional and nullable inInsertandUpdate. Properly aligned with the SQL schema.packages/database/supabase/schemas/sync.sql (2)
18-21: LGTM!Schema changes are appropriate:
last_task_startis correctly NOT NULL (every task must have a start time), whilelast_success_startis nullable since new tasks haven't completed successfully yet.
49-90: LGTM!The
end_sync_taskfunction correctly trackslast_success_start:
- On completion (line 74): captures the current task's start time as the last successful start
- On failure/timeout: preserves the existing
last_success_startvalue- The worker assertion (line 70) properly guards against task completion by the wrong worker
packages/database/supabase/migrations/20251216222945_sync_last_success_start.sql (3)
1-7: LGTM!The migration correctly:
- Adds the nullable
last_success_startcolumn- Enforces
NOT NULLonlast_task_start- Backfills
last_success_startfromlast_task_startfor rows withlast_task_endset (which indicates successful completion)The backfill condition is correct since
last_task_endis only populated whenend_sync_taskis called withstatus='complete'.
8-46: LGTM!The
end_sync_taskfunction correctly implements the state tracking for successful task completions. The logic properly updateslast_success_startonly when status is 'complete'.
49-120: LGTM!The
propose_sync_taskfunction now correctly returnst_last_success_start(line 98) when taking over an existing task, enabling callers to accurately track successful sync progress rather than just task attempts.
maparent
force-pushed
the
eng-1121-endsynctask-error-wrong-worker
branch
from
db88a16 to
b6d316f
Compare
https://linear.app/discourse-graphs/issue/ENG-1121/endsynctask-error-wrong-worker
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.